Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feat/historical v8 #328

Closed
wants to merge 5 commits into from
Closed

Feat/historical v8 #328

wants to merge 5 commits into from

Conversation

huned
Copy link

@huned huned commented Nov 14, 2021

Closes #238.

Changes

  • Apply patch generated from feat(historical2): add historical2 module #239 (originally authored by @PythonCreator27) to fetch historical prices, splits, dividends in one network request to the YF v8 chart api endpoint.
  • Rename module to _historicalv8 to indicate that it uses a YF v8 API and that it's not production-ready (the "_" prefix)
  • Add some basic documentation and tests, which still have three test failures. (See additional details below.)

Type

  • New Module
  • Other New Feature
  • Validation Fix
  • Other Bugfix
  • Docs
  • Chore/other

Comments/notes

Re test failures: At this time, there are three test failures for this new _historicalv8 module.

  • Symbol ADH: Fails schema validation. The data is contained in the JSON, so need to figure this out and update the schema.
  • Symbol BTC-USD: Fails schema validation. The data is contained in the JSON, so need to figure this out and update the schema.
  • Symbol MDLY: Yahoo Finance responds with a 404 not found. Notably, MDLY was delisted by NYSE so this seems correct.

@gadicc - any guidance on what I should do about the ADH, BTC-USD failures?

@PythonCreator27 - while I'm moving this forward, any other changes you suggest we make here to get this merge-able?

…equest.

1. Apply patch generated from [gadicc#239] that fetches
historical prices, splits, and dividends in a single network request.
Original code by @PythonCreator27.

2. Regenerate schema.

Fixes [gadicc/node-yahoo-finance#238]
1. Rename to indicate that this module it uses Yahoo Finance API v8.
Also prefix the filename with "_" to indicate that it's not fully
baked yet.

2. Regenerate schema.
1. Copied historical.spec.ts to _historicalv8.spec.ts and updated it.
   We use a year-long date range to fetch historical data so we are
   sure to at least get some dividends and splits. (E.g., see
   `tests/http/historicalv8-AAPL-2020-01-01-to-2021-01-04.json`)

2. At this time, there are three test failures for this new historicalv8
   module.
   - Symbol ADH: Fails schema validation.  The data is contained in the
     JSON, so need to figure this out and update the schema.
   - Symbol BTC-USD: Fails schema validation. The data is contained in
     the JSON, so need to figure this out and update the schema.
   - Symbol MDLY: Yahoo Finance responds with a 404 not found. Notably,
     [MDLY was delisted by NYSE](https://www.businesswire.com/news/home/20210707005906/en/NYSE-to-Suspend-Trading-Immediately-in-Medley-Management-Inc.-MDLY-and-Medley-LLC-MDLQ-and-MDLX-and-Commence-Delisting-Proceedings) so
     this seems correct. (Note to self: Yahoo Finance data exhibits
     survivorship bias.)
To indicate that it's a work-in-progress.
@huned
Copy link
Author

huned commented Nov 17, 2021

@gadicc I know you're probably busy but just pinging you in case you missed this.

@gadicc
Copy link
Owner

gadicc commented Nov 18, 2021

@huned thanks for the ping. Will do my best to review this in the coming days. Thanks for your patience! 🙏

@gadicc gadicc mentioned this pull request Nov 21, 2021
@gadicc
Copy link
Owner

gadicc commented Nov 21, 2021

Ok, @PythonCreator27, @huned; I realize this is going to hurt my popularity, especially after all the work you've both put in to this. But after a lot of thought on the matter, I've decided it's bad idea to create a historicalV8 module that tries to mimick historical but uses Yahoo's chart API under the hood. So, I've created a new _chart module (details should show up in the commit after this comment).

Upon doing so, I of course came across a lot of the issues you have both already addressed. In retrospect - and I do apologise - clearly simply asking you to rename the module and amend some of the docs would have been a better step, and I regret that I didn't realize this in the beginning.

In particular, I prefer the way @PythonCreator27 outputs the data (arrays with { date: ... } rather than Objects with timestamp keys). Originally I thought we could have a conversion module that relies on _chart, but ultimately realized the historicalV8 tries less harder than I thought to mimick the old historical's output and is just a lot more sensible than Yahoo's output.

So, let me put a little more thought into this... and again, I really do apologize at how I've handled this. Anyways, I'm going to push the changes now and we can have some discussion in a new issue which I'll link back to here too.

@gadicc
Copy link
Owner

gadicc commented Nov 21, 2021

@huned, regarding also the failing test symbols, quoting chart.spec.ts:

  const symbolsToSkip = [
    "ADH", // currency: null; Yahoo-finance does show a chart though, should we allow this?
    "BEKE", // BadRequestError: Data doesn't exist for startDate = 1577836800, endDate = 1578009600
    "BFLY", // BadRequestError: Data doesn't exist for startDate = 1577836800, endDate = 1578009600
    "^VXAPL", // firstTradeDate: null; Yahoo-finance shows an empty chart even though there's some data.
  ];

Thanks also for the heads up on MDL, I removed all of these in 0daef32.

@huned
Copy link
Author

huned commented Nov 21, 2021

Thanks @gadicc ... And I think everything you're saying is eminently sensible (and no need to apologize for it).

I also had a weird gut feeling about calling it historical so I think _chart makes better sense. Will take a look at your commits and provide feedback on the output format tomorrow.

Thanks again 💯

@huned huned closed this Nov 21, 2021
@gadicc
Copy link
Owner

gadicc commented Nov 22, 2021

Thanks, @huned!

And for your kind words. Nevertheless, I want my apology clearly on record for this one. I greatly value contributors' time and efforts on this project, which is a great help to me, and the community. I'm not in the habit of taking others' PR's and rewriting them as my own. In this case, it was only after finishing the code and then again looking back at this PR, that I really saw the similarity. On my next edit, I'll be adding a co-authored comment to the top of the file for the 3 of us.

Ok great, we'll chat about the rest in the other issue. Thanks again!

gadicc pushed a commit that referenced this pull request Dec 21, 2021
# [2.1.0](v2.0.1...v2.1.0) (2021-12-21)

### Bug Fixes

* **chart:** more query tests, intervals, edge cases ([#336](#336)) ([6b95d7e](6b95d7e))
* **package:** have semantic-release recognize version branches ([a89d895](a89d895))
* **quote:** equity: allow underlyingSymbol.  LDN.MI test ([#363](#363)) ([817410b](817410b))

### Features

* **chart:** { return: "array" } (default) + test fix ([#336](#336)) ([1ac66c3](1ac66c3))
* **chart:** initial release as "_chart" ([#239](#239), [#328](#328), [#334](#334)) ([92b90b1](92b90b1))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Stock splits and getting dividends, stock splits, and other historical data in one function call
2 participants